Conversation
| // This is used internally instead of Rf_eval() to make evaluation safer | ||
| inline SEXP Rcpp_eval_impl(SEXP expr, SEXP env) { | ||
| return Rcpp_fast_eval(expr, env); | ||
| inline SEXP Rcpp_eval_impl(SEXP expr, SEXP env) { // #nocov |
There was a problem hiding this comment.
Shouldn't we drop this and use Rcpp_fast_eval directly in the remaining spots?
There was a problem hiding this comment.
Well spotted. And when you step back and look at that file you see this in a 'else' branch we no longer need (as we're always greater or equal R 3.5.0 now) and with it the whole UNWIND_PROTECT define can likely go away. I had been meaning to ping you about that. Cleanup for the next release cycle?
|
|
||
| /* check if we can use a fast version */ | ||
| if( TYPEOF(x) == STRSXP && parent.size() == Rf_length(x) ){ | ||
| if( TYPEOF(x) == STRSXP && parent.size() == Rf_length(x) ){ // #nocov start |
There was a problem hiding this comment.
Maybe we should design a test to check the other side of the branch instead?
There was a problem hiding this comment.
In theory, just about every instance of #nocov could deserve a test. Some of this areas have been without one for twelve or more years though. Adding tests is always the hope, in practice it does not happen all that much.
| try{ | ||
| index = parent.offset(name) ; | ||
| parent[ index ] = rhs ; | ||
| parent[ index ] = rhs ; // #nocov |
There was a problem hiding this comment.
Weird. Does this mean that we don't have any test that doesn't throw here?
There was a problem hiding this comment.
Maybe. That is indeed one of the weirder ones.
| /* We need to evaluate if it is a promise */ | ||
| if( TYPEOF(res) == PROMSXP){ | ||
| res = internal::Rcpp_eval_impl( res, env ) ; | ||
| res = internal::Rcpp_eval_impl( res, env ) ; // #nocov |
There was a problem hiding this comment.
Looks like this is important enough to test it?
There was a problem hiding this comment.
Sure. PRs always welcome. Or if you want to add a quick test now we can use this PR.
| typedef XPtr<class_Base> XP_Class ; | ||
| typedef Rcpp::XPtr<Rcpp::Module> XP ; | ||
| CppClass( SEXP x) : S4(x){}; | ||
| CppClass( SEXP x) : S4(x){}; // #nocov |
There was a problem hiding this comment.
Maybe we should test instantiation from SEXP?
There was a problem hiding this comment.
Same as last comment.
|
@Enchufa2 Any more (blocking ?) comments? Can this proceed or do you feel it is not an improvement? It (in some cases) adds explicit #nocov to code lacking a test for over a decade, as do other spots: $ ag -c '#nocov' inst/include/ src/ R/ | awk -F: '{v += $2;} END { printf("Total count is %d\n", v) }'
Total count is 451
$ Ideally we add new tests. In practice we haven't. This PR adds a few more to the existing tags. No more, no less. Should not make anything 'worse'. |
Enchufa2
left a comment
Enchufa2
left a comment
There was a problem hiding this comment.
I might take a look at those things another time, but not now. The problem of increasing coverage based on nocov tags is that it obscures those potential problems, but no strong opinions about it. I'll grep for nocov and that's it.
|
I think that is a totally fair view. It's one of "those things" we could be / should be doing but there are often more pressing issues at hand. |
2 participants
This PR updates #nocov tags for a number of R and C++ source files. Apart from one remaining 'function factory' function that confuses the coverage counting, this pushes the needle towards full coverage.
Checklist
R CMD checkstill passes all tests